-
-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #579 #580
fix #579 #580
Conversation
sorry, this is wrong, it only result to un used import. |
Ok, only saw this after commenting on the draft PR. I'll check out that PR and see if I understand the issue. |
It can be fixed by changing |
It doesn't require that. There are two overloads for the edit: yeah, thinking about this it should be clarified somewhere if overloads of the kind: proc foo[T](x: T): T = discard
proc foo[T](x: Bar[T]): Bar[T] = discard should be "valid" in general, i.e. the compiler should always correctly deduce to use the |
as you can see there's little bit ambiguous, |
Yeah, the following does work: proc numerical_gradient*[T: not Tensor](input: T, f: (proc(x: T): T), h = T(1e-5)): T {.inline.} =
...
proc numerical_gradient*[T](input: Tensor[T], f: (proc(x: Tensor[T]): T), h = T(1e-5)): Tensor[T] {.noinit.} =
.... and no lost type information. But as I mention in my edit in my last message, I'm not sure if the compiler shouldn't be smart enough to figure out the correct overload itself? Why does it pick the edit again: Ah, of course. We don't have a direct |
Give me a few minutes to understand this... |
it should go |
yeah, that's hard to understand for me, as all the test calls use proc |
So: proc numerical_gradient*[T](input: Tensor[T], f: (proc(x: Tensor[T]): T), h = T(1e-5)): Tensor[T] {.noinit.} overload. However, at this point the other overload using only The lambda generated by The call As the only If however the compiler picks the overload proc numerical_gradient*[T](input: T, f: (proc(x: T): T), h = T(1e-5)): T {.inline.} first, the type signatures cannot be satisfied anymore, as the lambda So the breakage in the nim compiler seems to be the inability to
To me it seems like the issue is that previously the compiler picked Does this make any sense? |
In either case, clarifying the signature for the |
yes, exactly, your description is correct, I just tried |
Yeah, I agree. Will you add a test case for the behavior? |
yeah, I'll , it should be a test in nim repo not relys on package's |
fix #579